Conversation
…eject unknown TOML fields, add random API key generation Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/8c253e67-ea5f-4636-a9df-90086e1ce1e6 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…OML field detection Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/8c253e67-ea5f-4636-a9df-90086e1ce1e6 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
lpcox
left a comment
There was a problem hiding this comment.
Review
The three compliance fixes look correct:
isMalformedAuthHeader()— RFC 7230 null-byte/control-char check is soundisDynamicTOMLPath()— correctly exemptsguard_policiesandguards.*.configmap sections from unknown-field rejectiongenerateRandomAPIKey()— usescrypto/randproperly with 32-byte hex output
Conflict with PR #3098
This PR renames newMinimalUnifiedServer → newSessionTestUnifiedServer in session_test.go. PR #3098 (already pushed) fixes the same duplicate-declaration issue by renaming it to newMinimalUnifiedServerForSessionTest. Whichever merges second will conflict — please coordinate or rebase after #3098 lands to avoid a merge failure.
Nit: TestRequireSession duplicate
Same situation — this PR removes the duplicate TestRequireSession from session_test.go, while #3098 renames it to TestRequireSession_SessionManagement. Either approach works; just needs one to merge first.
There was a problem hiding this comment.
Pull request overview
This PR addresses several MCP Gateway spec compliance gaps by tightening authentication error handling, enforcing strict TOML schema validation, and ensuring API key auth is always enabled via startup key generation (plus resolving test/build conflicts in the server package).
Changes:
- Return HTTP 400 (instead of 401) for malformed
Authorizationheaders by detecting RFC 7230-invalid control characters. - Reject unknown TOML fields during
LoadFromFile(with exemptions for intentionally dynamicmap[string]interface{}sections). - Auto-generate a cryptographically random API key at startup when none is configured, and propagate it to both auth and stdout config output; update tests accordingly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/binary_test.go | Writes a known gateway.api_key into temp TOML so integration tests can authenticate deterministically. |
| internal/server/unified_test.go | Removes a superseded/duplicate requireSession test to fix build/test conflicts. |
| internal/server/session_test.go | Renames session-test helper to avoid duplicate helper name/signature conflicts. |
| internal/server/auth.go | Adds malformed header detection and returns 400 for invalid Authorization header values. |
| internal/server/auth_test.go | Adds test coverage for malformed Authorization header handling and the helper. |
| internal/config/config_test.go | Updates TOML unknown-key tests to assert hard failure instead of warning-only behavior. |
| internal/config/config_core.go | Enforces unknown-field rejection using MetaData.Undecoded() with dynamic-path exemptions. |
| internal/config/config_core_test.go | Updates unknown-key test expectations for new strict TOML behavior. |
| internal/cmd/root.go | Generates and installs a random API key into config at startup when missing. |
| internal/cmd/root_test.go | Adds unit test coverage for random API key generation helper. |
| docs/CONFIGURATION.md | Updates documentation to reflect unknown-field rejection behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if cfg.GetAPIKey() == "" { | ||
| randomKey, err := generateRandomAPIKey() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to generate random API key: %w", err) |
There was a problem hiding this comment.
generateRandomAPIKey() already wraps rand.Read errors with "failed to generate random API key". The caller in run() wraps the returned error with the same prefix again, which will produce duplicated text like "failed to generate random API key: failed to generate random API key: …". Consider returning the raw rand.Read error from generateRandomAPIKey (or removing the extra wrap in run()) so the error chain is not repetitive.
| return fmt.Errorf("failed to generate random API key: %w", err) | |
| return err |
| @@ -182,12 +182,11 @@ prot = 3000 | |||
| command = "docker" | |||
| args = ["run", "--rm", "-i", "ghcr.io/github/github-mcp-server:latest"] | |||
| `) | |||
| // Unknown key "prot" (typo for "port") should warn but not error | |||
| // Unknown key "prot" (typo for "port") must now return an error per spec §4.3.1 | |||
| cfg, err := LoadFromFile(path) | |||
| require.NoError(t, err) | |||
| require.NotNil(t, cfg) | |||
| // Port should use default since "prot" was not recognized | |||
| assert.Equal(t, DefaultPort, cfg.Gateway.Port) | |||
| require.Error(t, err) | |||
| assert.Nil(t, cfg) | |||
| assert.Contains(t, err.Error(), "unrecognized field") | |||
There was a problem hiding this comment.
The test name TestLoadFromFile_UnknownKeysDoNotCauseError no longer matches the behavior/assertions (it now expects LoadFromFile to return an error on unknown keys). Renaming the test to reflect the new contract (e.g., TestLoadFromFile_UnknownKeysCauseError) would avoid confusion and keep intent clear.
Three spec compliance violations from the daily compliance review (§7.2, §4.3.1, §7.3) plus a pre-existing server package build failure.
Pre-existing build fix
session_test.goconflicted withensure_guard_initialized_test.go(duplicatenewMinimalUnifiedServerwith different signatures) andunified_test.go(duplicateTestRequireSession). Resolved by renaming the helper and removing the superseded test.HTTP 400 for malformed Authorization headers (spec §7.2)
authMiddlewarepreviously returned 401 for all failures. AddedisMalformedAuthHeader()that rejects null bytes and control characters (RFC 7230) with HTTP 400 before the key comparison:Reject unknown TOML fields (spec §4.3.1)
md.Undecoded()results were previously only logged as warnings. Now returns a fatal error with a user-friendly message listing the offending field paths. AddedisDynamicTOMLPath()to exempt intentionally free-formmap[string]interface{}sections (servers.*.guard_policies.*.*andguards.*.config.*) which the TOML library reports as undecoded even when correctly parsed.Updated all tests that previously asserted unknown keys were silently accepted.
Random API key auto-generation (spec §7.3)
When no
api_keyis configured, the gateway now generates a cryptographically random 32-byte hex key at startup, stores it incfg.Gateway.APIKey, and propagates it to both the auth middleware and the stdout config output (spec §5.4). Integration tests updated to include an explicitapi_keyso they authenticate against the now-always-enabled middleware.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build1514168667/b336/launcher.test /tmp/go-build1514168667/b336/launcher.test -test.testlogfile=/tmp/go-build1514168667/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go da1ZMnT7I p/bin/git pull.rebase(dns block)/tmp/go-build2448957059/b336/launcher.test /tmp/go-build2448957059/b336/launcher.test -test.testlogfile=/tmp/go-build2448957059/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 4fyi/m7bmpbpAhnIBg0bf4fyi -I /home/REDACTED/.cargo/bin/as --gdwarf-5 --64 -o 4168667/b240/imp-buildtags -I ue pkg/mod/github.c-ifaceassert 64/pkg/tool/linu-nilfunc --gdwarf-5 --64 -o 64/pkg/tool/linu{{.Config.OpenStdin}}(dns block)/tmp/go-build4183996463/b336/launcher.test /tmp/go-build4183996463/b336/launcher.test -test.testlogfile=/tmp/go-build4183996463/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true --version ache/go/1.25.8/x64/pkg/tool/linu--others /usr/libexec/docker/docker-init esXM/51DG9sgD9v0bash -I 64/pkg/tool/linu--version /usr/libexec/docker/docker-init --ve�� -o 64/pkg/tool/linux_amd64/link ker/docker-init d -n 10 pkg/mod/github.cdocker-cli-plugin-metadata x_amd64/compile ker/docker-init(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build1514168667/b318/config.test /tmp/go-build1514168667/b318/config.test -test.testlogfile=/tmp/go-build1514168667/b318/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -c=4 -nolocalimports -importcfg /tmp/go-build1514168667/b196/importcfg -pack /home/REDACTED/go/pkg/mod/github.com/stretchr/testify@v1.11.1/require/doc.go /home/REDACTED/go/pkg/mod/github.com/stretchr/testify@v1.11.1/require/forward_requirements.go ortc�� go 64/src/internal/runtime/strconv/--gdwarf2 u/13/cc1 user.name obyte odules/npm/node_-bool u/13/cc1(dns block)/tmp/go-build2448957059/b318/config.test /tmp/go-build2448957059/b318/config.test -test.testlogfile=/tmp/go-build2448957059/b318/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -c=4 -nolocalimports -importcfg /tmp/go-build1636263839/b001/importcfg -pack /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/cmd/completion.go /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/cmd/flags.go ortc�� 4168667/b244/_pkg_.a stmain.go docker-compose . b/gh-aw-mcpg/int--norc --64 docker-compose(dns block)/tmp/go-build2141364385/b318/config.test /tmp/go-build2141364385/b318/config.test -test.testlogfile=/tmp/go-build2141364385/b318/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -bool -buildtags x_amd64/compile -errorsas -ifaceassert -nilfunc x_amd64/compile -o /tmp/go-build1514168667/b345/_pk-p -trimpath docker-buildx -p main -lang=go1.25 docker-buildx(dns block)nonexistent.local/tmp/go-build1514168667/b336/launcher.test /tmp/go-build1514168667/b336/launcher.test -test.testlogfile=/tmp/go-build1514168667/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go da1ZMnT7I p/bin/git pull.rebase(dns block)/tmp/go-build2448957059/b336/launcher.test /tmp/go-build2448957059/b336/launcher.test -test.testlogfile=/tmp/go-build2448957059/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 4fyi/m7bmpbpAhnIBg0bf4fyi -I /home/REDACTED/.cargo/bin/as --gdwarf-5 --64 -o 4168667/b240/imp-buildtags -I ue pkg/mod/github.c-ifaceassert 64/pkg/tool/linu-nilfunc --gdwarf-5 --64 -o 64/pkg/tool/linu{{.Config.OpenStdin}}(dns block)/tmp/go-build4183996463/b336/launcher.test /tmp/go-build4183996463/b336/launcher.test -test.testlogfile=/tmp/go-build4183996463/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true --version ache/go/1.25.8/x64/pkg/tool/linu--others /usr/libexec/docker/docker-init esXM/51DG9sgD9v0bash -I 64/pkg/tool/linu--version /usr/libexec/docker/docker-init --ve�� -o 64/pkg/tool/linux_amd64/link ker/docker-init d -n 10 pkg/mod/github.cdocker-cli-plugin-metadata x_amd64/compile ker/docker-init(dns block)slow.example.com/tmp/go-build1514168667/b336/launcher.test /tmp/go-build1514168667/b336/launcher.test -test.testlogfile=/tmp/go-build1514168667/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go da1ZMnT7I p/bin/git pull.rebase(dns block)/tmp/go-build2448957059/b336/launcher.test /tmp/go-build2448957059/b336/launcher.test -test.testlogfile=/tmp/go-build2448957059/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 4fyi/m7bmpbpAhnIBg0bf4fyi -I /home/REDACTED/.cargo/bin/as --gdwarf-5 --64 -o 4168667/b240/imp-buildtags -I ue pkg/mod/github.c-ifaceassert 64/pkg/tool/linu-nilfunc --gdwarf-5 --64 -o 64/pkg/tool/linu{{.Config.OpenStdin}}(dns block)/tmp/go-build4183996463/b336/launcher.test /tmp/go-build4183996463/b336/launcher.test -test.testlogfile=/tmp/go-build4183996463/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true --version ache/go/1.25.8/x64/pkg/tool/linu--others /usr/libexec/docker/docker-init esXM/51DG9sgD9v0bash -I 64/pkg/tool/linu--version /usr/libexec/docker/docker-init --ve�� -o 64/pkg/tool/linux_amd64/link ker/docker-init d -n 10 pkg/mod/github.cdocker-cli-plugin-metadata x_amd64/compile ker/docker-init(dns block)this-host-does-not-exist-12345.com/tmp/go-build1514168667/b345/mcp.test /tmp/go-build1514168667/b345/mcp.test -test.testlogfile=/tmp/go-build1514168667/b345/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/c-I --local ache/go/1.25.8/x-I gpg.program abis(dns block)/tmp/go-build2448957059/b345/mcp.test /tmp/go-build2448957059/b345/mcp.test -test.testlogfile=/tmp/go-build2448957059/b345/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -bool -buildtags ndor/bin/bash -errorsas -ifaceassert -nilfunc /opt/hostedtoolc-tests -ato�� -bool -buildtags /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/link -errorsas -ifaceassert -nilfunc /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/link(dns block)/tmp/go-build4183996463/b345/mcp.test /tmp/go-build4183996463/b345/mcp.test -test.testlogfile=/tmp/go-build4183996463/b345/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -bool -buildtags iginal -errorsas -ifaceassert -nilfunc iginal -slo�� -stringintconv -tests .13/x64/bash -bool din}} /opt/hostedtoolc(create|run) ker/cli-plugins/docker-buildx(dns block)If you need me to access, download, or install something from one of these locations, you can either: